Skip to content

Conversation

Lms24
Copy link
Member

@Lms24 Lms24 commented May 20, 2025

This PR turns on the explicit-function-return-type eslint rule. I noticed in #741 that we tended to export public API with implicit function return types (and sometimes even declared types dynamically from implicit function return types). This has high potential to introduce accidental breaking changes. While I rewrote the biggest offenders in #741, this PR now turns on an eslint rule to enforce explicit function return types.

As a result of the eslint rule, I had to turn add some return types but most of them are very straight forward. I'll leave comments on things that need special attention.

@Lms24 Lms24 self-assigned this May 20, 2025
@Lms24 Lms24 requested review from a team, mydea and andreiborza and removed request for a team May 20, 2025 12:40
@Lms24
Copy link
Member Author

Lms24 commented May 20, 2025

hmm looks like I broke some tests 😕 -- setting to draft for now

@Lms24 Lms24 marked this pull request as draft May 20, 2025 12:42
@Lms24 Lms24 force-pushed the lms/ref-eslint-explicit-function-return-type branch from 6c7c1ea to fb03f50 Compare May 20, 2025 14:32
}
// TODO(v4): Don't export this from the package but keep the utils version
// eslint-disable-next-line @typescript-eslint/explicit-function-return-type
export const getBuildInformation = actualGetBuildInformation;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We had 1:1 the same implementation in the actualGetBuildInformation util function, so I just removed the duplication (but kept the top level export deprecated).

Comment on lines +270 to +287
/**
* Simplified `renderChunk` hook type from Rollup.
* We can't reference the type directly because the Vite plugin complains
* about type mismatches
*/
type RenderChunkHook = (
code: string,
chunk: {
fileName: string;
}
) => {
code: string;
map: SourceMap;
} | null;

export function createRollupDebugIdInjectionHooks(): {
renderChunk: RenderChunkHook;
} {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the comment explains this but maybe just to add: I can't use UnpluginOptions["renderChunk"]; because renderChunk is not a hook known to Unplugin. We manually pass in this hook in a plugin specifically for Rollup and Vite.

Comment on lines +365 to +369
writeBundle: (
outputOptions: { dir?: string; file?: string },
bundle: { [fileName: string]: unknown }
) => Promise<void>;
} {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same for writeBundle as for renderChunk (see comment above)

@Lms24 Lms24 marked this pull request as ready for review May 20, 2025 15:17
Copy link
Member

@andreiborza andreiborza left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for improving this!

@Lms24 Lms24 merged commit c6b18cd into main May 21, 2025
23 checks passed
@Lms24 Lms24 deleted the lms/ref-eslint-explicit-function-return-type branch May 21, 2025 10:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants